Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ImageData Float16Array support #11143

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ccameron-chromium
Copy link
Contributor

@ccameron-chromium ccameron-chromium commented Mar 19, 2025

Add the ImageDataPixelFormat enum type, which can take on values of "rgba-unorm8" (the current default of 8-bit unsigned normalized RGBA) and "rgba-float16" (the new option for 16-bit floating point RGBA).

Add ImageDataPixelFormat to the ImageDataSettings dictionary used by ImageData constructors, to allow creation of ImageData objects that are backed by Float16Array.

Add ImageDataPixelFormat as an attribute to ImageData.

Add a new ImageDataArray type which is the union of Uint8ClampedArray and Float16Array. Change the ImageData data attribute to be this union type instead of Uint8ClampedArray.

Fix the Canvas Pixel ArrayBuffer section to correctly describe the pixel layout of an ImageBuffer (there was a bug in the spec specifying that an ImageData's pixel data starts at the beginning of its data attributes's backing ArrayBuffer, when what was intended was for it to start at the beginning of its data attribute).

Fixes #10856

(See WHATWG Working Mode: Changes for more details.)


/canvas.html ( diff )
/infrastructure.html ( diff )

Add the ImageDataPixelFormat enum type, which can take on values of
"rgba-unorm8" (the current default of 8-bit unsigned normalized RGBA)
and "rgba-float16" (the new option for 16-bit floating point RGBA).

Add ImageDataPixelFormat to the ImageDataSettings dictionary used by
ImageData constructors, to allow creation of ImageData objects that are
backed by Float16Array.

Add ImageDataPixelFormat as an attribute to ImageData.

Add a new ImageDataArray type which is the union of Uint8ClampedArray
and Float16Array. Change the ImageData data attribute to be this union
type instead of Uint8ClampedArray.

Fix the Canvas Pixel ArrayBuffer section to correctly describe the pixel
layout of an ImageBuffer (there was a bug in the spec specifying that an
ImageData's pixel data starts at the beginning of its data attributes's
backing ArrayBuffer, when what was intended was for it to start at the
beginning of its data attribute).

Fixes whatwg#10856
@@ -70609,30 +70614,61 @@ try {
<p>To <dfn>initialize an <code>ImageData</code> object</dfn> <var>imageData</var>, given a
positive integer number of rows <var>rows</var>, a positive integer number of pixels per row
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't new, but aren't the arguments inverted here?
All the call-sites do Initialize imageData given sw, sh, settings set to [...], but here the algorithm expects number of rows as the first argument and then pixels per row.
Though maybe this should be fixed separately and I'm sure there is some modernization that could get in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I swapped the ordering of the parameters, since that seems fairly urgent to not have wrong (when we do a modernization pass, we should also change the names to width and height).

@domfarolino
Copy link
Member

Add ImageDataPixelFormat as an attribute to ImageData.

This PR also adds the pixelFormat readonly attribute to ImageData, but I'm not completely sure why. The explainer indicates that it is for feature detection, but couldn't you just as easily type-check the underlying data? Is this member just added as a convenience mechanism for that, or is it necessary for some other reason? My concern is that it isn't necessary, but that it does add complexity just to validate/synchronize the type of the input source with the pixelFormat descriptor in the settings object.

I could imagine this being important if you wanted to accept a Float16Array yet force it to be treated like a Uint8ClampedArray by passing in pixelFormat = "rgba-unorm8", but we don't seem to support that.

@ccameron-chromium
Copy link
Contributor Author

Add ImageDataPixelFormat as an attribute to ImageData.

This PR also adds the pixelFormat readonly attribute to ImageData, but I'm not completely sure why. [...] My concern is that it isn't necessary, but that it does add complexity just to validate/synchronize the type of the input source with the pixelFormat descriptor in the settings object.

Indeed this is partially for future-proofing. This is intended to handle the situation where there isn't a 1:1 relationship between the TypedArray type and the actual pixel format (for now, there is indeed a 1:1 relationship). A simple example (but not the most useful) is that one could have an rg-unorm8 format, which also uses Uint8ClampedArray, but only has two channels (red-green). There could also be things like grey-alpha as well, etc.

It's also there because it felt odd for the ImageDataSettings to have a pixelFormat for use by constructors that would then disappear and not be query-able.

And it's also a good way of checking the actual layout (as opposed to the sub-type of the ArrayBuffer). Cause it will always be the case that pixelFormat implies a specific ArrayBuffer type.

(And feature detection was sort of an afterthought).

I could imagine this being important if you wanted to accept a Float16Array yet force it to be treated like a Uint8ClampedArray by passing in pixelFormat = "rgba-unorm8", but we don't seem to support that.

Indeed, we would not want to support anything like that (float to uint8, etc).

@domfarolino
Copy link
Member

It's also there because it felt odd for the ImageDataSettings to have a pixelFormat for use by constructors that would then disappear and not be query-able.

Right, I think it's a reasonable member of ImageData if it needs to be passed in via settings—it was just the passing in via settings that I was questioning in the first place. I think your explanation of different pixel format types mapping to the same underlying ArrayBuffer type makes sense and is compelling, thanks!

data-x="idl-Uint8ClampedArray">Uint8ClampedArray</code> argument, interpreted using the given
dimensions and the color space indicated by <var>settings</var>.</p>
<p>Returns an <code>ImageData</code> object using the data provided in the <code>ImageDataArray</code>
argument, interpreted using the given dimensions and the color space indicated by <var>settings</var>.</p>

<p>As each pixel in the data is represented by four numbers, the length of the data needs to be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this need updating as well?

@@ -70491,7 +70497,7 @@ try {
constructor steps are:</p>

<ol>
<li><p>Let <var>length</var> be the number of bytes in <var>data</var>.</p></li>
<li><p>Let <var>length</var> be the length of <var>data</var>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should reference some actual algorithm. It seems currently we only have https://webidl.spec.whatwg.org/#buffersource-byte-length so maybe we need to add this to Web IDL first.

<li><p>If <var>source</var> was given, then initialize the <dfn attribute for="ImageData"><code
data-x="dom-imagedata-data">data</code></dfn> attribute of <var>imageData</var> to
<var>source</var>.</p></li>
<li><p>If <var>source</var> was given then:</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the <li> contains multiple children the <p> needs to be on a new line.

Also no need for "then" if it's followed by a set of nested steps.

<var>source</var>.</p></li>
<li><p>If <var>source</var> was given then:</p>
<ol>
<li><p>If <var>settings</var> was given and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this is indented too much.

<var>settings</var>["<dfn dict-member for="ImageDataSettings"><code
data-x="dom-ImageDataSettings-pixelFormat">pixelFormat</code></dfn>"].</p></li>

<li><p>Otherwise, initialize the <code data-x="dom-imagedata-pixelFormat">pixelFormat</code>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not add new initialization of attributes. We should instead define internal fields we can manipulate here and that the getter ends up returning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

ImageData: Add Float16Array backing support
4 participants